Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ldml_test] add experimental/sil/ldml_test #2865

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

[ldml_test] add experimental/sil/ldml_test #2865

wants to merge 2 commits into from
+478 −0

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Jun 28, 2024 •
edited by mcdurdin
Loading

@srl295 srl295 self-assigned this Jun 28, 2024
@srl295 srl295 mentioned this pull request Jun 28, 2024
Closed
@DavidLRowe
Copy link
Contributor

The convention for the keyboards repository is to store the keyboard in a folder based on the first letter or prefix, so:
release/s/sample
release/sil/sil_ipa

Following that convention, I'd expect
experimental/l/ldml_test OR
experimental/sil/sil_ldml_test

@srl295
Copy link
Member Author

srl295 commented Jun 28, 2024

The main point was to test ldml. I expected there may be updates needed for local conventions.

Would sil_ldml_test make sense? I can rename it.

@DavidLRowe
Copy link
Contributor

sil/sil_ldml_test would fit the pattern.

Is this intended to be an example? If so, sil_ldml_example maybe?
Or is this the first of a suite of test keyboards? maybe a new folder "test": test/test_ldml (with test_ldml_xxx to follow)?

The concern is that current policy is to not change keyboard folder/project names once they've been created.

@srl295
Copy link
Member Author

srl295 commented Jun 28, 2024

It's more of a test than an example.

mcdurdin
mcdurdin reviewed Jun 30, 2024
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider marking this keyboard as deprecated just to avoid it coming up for end users in the keyboard search?

As @DavidLRowe noted, it needs to be called either experimental/sil/sil_ldml_test or experimental/l/ldml_test.

Looking good though. Seems to be putting CI through its paces

experimental/sil/ldml_test/source/ldml_test.kps
<Name URL="">LDML Test</Name>
<Copyright URL="">Copyright © Steven R. Loomis</Copyright>
<Author URL="">Steven R. Loomis</Author>
<Description URL="">Test Keyboard</Description>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inadequate detail because this is what will show in the keyboard details on keyman.com. We need this to be clear for end users who encounter it on keyman.com.

experimental/sil/ldml_test/README.md
Test Keyboard for LDML


Trying it Out
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this information would be better placed in welcome.htm -- then it is available when the keyboard is installed, and not just in the source.

experimental/sil/ldml_test/.gitignore
Comment on lines +1 to +2
/build
/*.kpj.user
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should not be needed as both of these are in the root-level .gitignore

@mcdurdin
Copy link
Member

From the build log:

10:56:14   ldml_test.kpj - info KM05002: Building experimental/sil/ldml_test/ldml_test.kpj
10:56:14   ldml_test.xml - info KM05002: Building experimental/sil/ldml_test/source/ldml_test.xml
10:56:14   ldml_test.xml - info KM05006: experimental/sil/ldml_test/source/ldml_test.xml built successfully.
10:56:14   ldml_test.kps - info KM05002: Building experimental/sil/ldml_test/source/ldml_test.kps
10:56:14   ldml_test.kps - info KM05006: experimental/sil/ldml_test/source/ldml_test.kps built successfully.
10:56:14   ldml_test.kpj - info KM05002: Building experimental/sil/ldml_test/ldml_test.kpj
10:56:14   ldml_test.kpj - info KM05006: experimental/sil/ldml_test/ldml_test.kpj built successfully.
10:56:14   ldml_test.kpj - info KM0500B: Project experimental/sil/ldml_test/ldml_test.kpj built successfully.

@srl295
Copy link
Member Author

srl295 commented Jul 1, 2024

I like the deprecated idea. Will update this.

@srl295
Copy link
Member Author

srl295 commented Jul 1, 2024

How do we mark it deprecated?

@DavidLRowe
Copy link
Contributor

The usual way this is done is for a keyboard to mark another keyboard as deprecated. In Keyman Developer (v17+) this can be done in the Packaging section, Details tab:
image
Use Add to add a related package and tick the Deprecated box to indicate that this related package should be deprecated.

@mcdurdin Is is possible for a package to deprecate itself?

@srl295 srl295 requested a review from mcdurdin July 4, 2024 21:24
@mcdurdin
Copy link
Member

mcdurdin commented Jul 4, 2024

srl295 requested a review from mcdurdin 1 hour ago

Still got some changes requested (plus this is in draft, so not clear if you intended to ask for review?)

@mcdurdin Is is possible for a package to deprecate itself?

I don't know! It'd be weird, and could have other side-effects, so I don't think I'm keen on doing that. So... despite my brilliant idea, I don't think we can go this way for now.

That is a weakness in the package-deprecation pattern, but it's not an easy one to solve without repetition of data... (DEPRECATED.md is informative only at this point, but we could test for its as a possibility?) Would add complexity to the keyboards build.

@mcdurdin mcdurdin mentioned this pull request Jul 4, 2024
Open
@mcdurdin
Copy link
Member

mcdurdin commented Jul 4, 2024

That is a weakness in the package-deprecation pattern, but it's not an easy one to solve without repetition of data... (DEPRECATED.md is informative only at this point, but we could test for its as a possibility?) Would add complexity to the keyboards build.

@srl295
Copy link
Member Author

srl295 commented Jul 5, 2024 via email •
edited
Loading

@mcdurdin mcdurdin mentioned this pull request Jul 5, 2024
Closed
@mcdurdin
Copy link
Member

I think we can probably close this as redundant given #2993 is a real LDML keyboard, so I think we don't need a test keyboard in the keyboards repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdurdin mcdurdin Awaiting requested review from mcdurdin

At least 1 approving review is required to merge this pull request.

Assignees

@srl295 srl295

Labels
None yet
Projects
Keyboards
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: add an experimental LDML keyboard to validate CI (on staging branch)
3 participants
@srl295 @DavidLRowe @mcdurdin